Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common: include: Redo some includes for FreeBSD #15337

Merged
merged 2 commits into from May 29, 2017

Conversation

wjwithagen
Copy link
Contributor

@wjwithagen wjwithagen commented May 27, 2017

  • During include cleanup just a too much was removed for FreeBSD
    to get things compiled.
    This redoes some of the includes.

Tracker: http://tracker.ceph.com/issues/19883
Signed-off-by: Willem Jan Withagen wjw@digiware.nl

@wjwithagen wjwithagen changed the title build/include: Redo som includes for FreeBSD build/include: Redo some includes for FreeBSD May 27, 2017
@wjwithagen wjwithagen requested a review from joscollin May 27, 2017 18:35
Copy link
Member

@joscollin joscollin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Could you please verify whether this PR follows the naming conventions for the PR Title and Commit Title ? They need change.
    Please refer: https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes

  2. My PRs for "Removing Redundant Headers" affects only the files inside src/common/ directory. But this PR contains "erasure-code" and "test" submodules and these are not re-added for sure. I usually create a separate PR for each submodule and especially the ones that has Separate logical changes (here: newly added, not reverted). You could consider that if it is not a hard for you.

  3. If this PR does a revert, then please add the Tracker URL in the Commit Message and in the PR Description.

  4. Update the tracker with the PR URLs.

#include <unistd.h>
#if defined(__FreeBSD__)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any problem making it similar ? I mean putting an #ifdef, which is similar to the other checks (linux)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joscollin
I've been using #if defined() for all my submissions.
This will allow me an easy grep to find all typical FreeBSD stuff.
And in the whole code there is a mismatch of all the possible forms.


#include <arpa/inet.h>
#include <ifaddrs.h>
#include <stdlib.h>
#include <string.h>
#if defined(__FreeBSD__)
#include <sys/types.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just noticed that this is newly added. But It is fine, if it is necessary in FreeBSD.

@@ -15,6 +15,9 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#if defined(__FreeBSD__)
#include <sys/wait.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just noticed that this is newly added. But It is fine, if it is necessary in FreeBSD.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joscollin
I'm not quite sure why these would be newly added.
The older version I have does have this include.
It even works without the #if

@wjwithagen wjwithagen changed the title build/include: Redo some includes for FreeBSD common: include: Redo some includes for FreeBSD May 28, 2017
@wjwithagen
Copy link
Contributor Author

@joscollin

I'm undoing more than just your reorganisation. Hence the erasure code patch.
I can make 2 different PRs if you prefer.

And I'll do the tracker administration.

- During include cleanup just a too bit much was removed for FreeBSD
  to get things compiled.
  This redoes some of the includes.

Tracker: http://tracker.ceph.com/issues/19883
Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
 - Buffer.h is needed to prevent Clang seriously complaining
   about missing and wrong forward declarations of
	ceph::buffer:ptr
   including buffer_fwd.h does not help.

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
@wjwithagen wjwithagen force-pushed the wip-wjw-freebsd-fix-includes branch from 4313f08 to 2f1b0c1 Compare May 28, 2017 12:57
@liewegas
Copy link
Member

@liewegas liewegas merged commit 0f803c5 into ceph:master May 29, 2017
@wjwithagen wjwithagen deleted the wip-wjw-freebsd-fix-includes branch January 23, 2019 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants